Skip to content

Conversation

@jaclync
Copy link
Contributor

@jaclync jaclync commented Oct 1, 2025

Fixes WOOMOB-1333

Description

This PR refactors the POSCatalogSyncCoordinator to ensure consistency between full and incremental sync interface and implementations by introducing private shouldPerform*Sync helper methods for both sync types.

Discussion ref: #16117 (comment)

Key Changes:

  • Added private shouldPerformFullSync method to encapsulate sync decision logic for full syncs
  • Made shouldPerformIncrementalSync private and consistent with full sync pattern
  • Updated both performFullSyncIfApplicable and performIncrementalSyncIfApplicable to accept maxAge and forceSync parameters
  • Removed maxIncrementalSyncAge property in favor of passing maxAge as a parameter
  • Added protocol extension with convenience methods performFullSync and performIncrementalSync that call the main methods with default parameters
  • Both sync methods now have consistent parameter signatures: siteID, maxAge, and forceSync

Benefits:

  • Consistent API surface between full and incremental sync operations
  • Better encapsulation of sync decision logic
  • More flexible for future analytics or UI integration (methods can be made public if needed)
  • Clearer separation between sync execution and sync decision logic

Steps to reproduce

Local Catalog is behind a local feature flag enabled in debug builds. No behavior changes are expected, a confidence testing would be helpful:

  • Launch the app in a store eligible for POS with catalog size <= 1000 --> observe the POSCatalogSyncCoordinator messages in the console, they should match the full sync status
  • Tap on the POS tab
  • Go to POS settings > Catalog --> the sync info should be up-to-date
  • Tap Refresh catalog to trigger a full sync --> after the full sync, the sync info should be up-to-date

Testing information

I tested for POS stores in iPad A16 iOS 26.0 simulator:

  • Catalog size <= 1000, no previous full sync
  • Catalog size <= 1000, force full sync
  • Catalog size <= 1000, previous full sync
  • Catalog size > 1000 --> skipped

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@jaclync jaclync changed the title [Woo POS][Local Catalog] Update full/incremental sync to have a private 'shouldPerform' for consistency [Local Catalog] Update full/incremental sync to have consistent interface Oct 1, 2025
@jaclync jaclync added type: technical debt Represents or solves tech debt of the project. feature: POS labels Oct 1, 2025
@jaclync jaclync added this to the 23.4 milestone Oct 1, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 1, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16187-ea8efcb
Version23.4
Bundle IDcom.automattic.alpha.woocommerce
Commitea8efcb
Installation URL4ln6quk98aqp0
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jaclync jaclync requested a review from joshheald October 1, 2025 07:37
@staskus staskus modified the milestones: 23.4, 23.5 Oct 3, 2025
Copy link
Contributor

@joshheald joshheald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected, I tried on an iPad Air with iOS 18. A suggestion inline about removing the forceSync parameter – take it or leave it though...

Also some tests need renaming

func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool
/// - forceSync: Whether to bypass age checks and always sync
/// - Throws: POSCatalogSyncError.syncAlreadyInProgress if a sync is already running for this site
func performFullSyncIfApplicable(for siteID: Int64, maxAge: TimeInterval, forceSync: Bool) async throws
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you anticipate forceSync doing more than bypassing age checks? If not, perhaps we can just pass maxAge: 0 when we want to force it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion - nope, it's just used to skip the age check now. In 149167f, I removed this parameter and added non-negative maxAge validation and only fetch last incremental date from database for age check for positive maxAge in shouldPerformIncrementalSync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a few of the old test names need to be updated from shouldPerformFullSync to performFullSync here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I went through the test cases and renamed them in 149167f.

@jaclync jaclync enabled auto-merge October 7, 2025 06:12
@jaclync jaclync merged commit 1d4ebce into trunk Oct 7, 2025
13 checks passed
@jaclync jaclync deleted the feat/WOOMOB-1333-catalog-sync-updates branch October 7, 2025 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS type: technical debt Represents or solves tech debt of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants